Replace outputstream with writeroutputstream #234

doniwinata0309 · 2020-11-02T09:58:04Z

Issue:
#233

use WriterOutputStream from apache common

transfuse/src/test/java/org/androidtransfuse/gen/FilerResourceWriterTest.java

transfuse-core/pom.xml

johncarl81 · 2020-11-03T22:08:13Z

transfuse-core/src/main/java/org/androidtransfuse/gen/FilerResourceWriter.java

-        OutputStream os = resource.openOutputStream();
-        openStreams.add(os);
-
+        OutputStream os = new WriterOutputStream(resource.openWriter());


This constructor is depreciated, we should define a charset.

I changed it to
OutputStream os = new WriterOutputStream(sourceFile.openWriter(), "UTF-8");

is it okay to use UTF-8 ?

johncarl81 · 2020-11-03T22:08:51Z

transfuse-core/src/main/java/org/androidtransfuse/gen/FilerResourceWriter.java

-        for (OutputStream openStream : openStreams) {
-            openStream.flush();
-            openStream.close();
-        }


Why not keep these calls? It seems like they would be necessary because we'll be dealing with a buffer with WriterOutputStream.

it is causing this error

Caused by: java.io.IOException: Stream closed at sun.nio.cs.StreamEncoder.ensureOpen (StreamEncoder.java:45) at sun.nio.cs.StreamEncoder.flush (StreamEncoder.java:140) at java.io.OutputStreamWriter.flush (OutputStreamWriter.java:229) at java.io.FilterWriter.flush (FilterWriter.java:100) at org.androidtransfuse.util.apache.commons.WriterOutputStream.flush (WriterOutputStream.java:83) at org.androidtransfuse.gen.FilerSourceCodeWriter.close (FilerSourceCodeWriter.java:65) at com.sun.codemodel.JCodeModel.build (JCodeModel.java:312)

Seems it already closed by the WriterOutputStream

public void close() throws IOException { this.processInput(true); this.flushOutput(); this.writer.close(); }

Do you have other clue why it causing error ?

Ah, I bet codemodel is already closing the output stream and since it's closed flush throws an error. You can outright get rid of the flush call instead of just commenting it out.

yes it seems already flush and also closed from the log, also looking from the flamegraph.

johncarl81 · 2020-11-03T22:09:19Z

transfuse/src/test/java/org/androidtransfuse/gen/FilerSourceCodeWriterTest.java

-
-        codeWriter.close();
-        verify(mockOutputStream).flush();
-        verify(mockOutputStream).close();


In general, I'd really like to keep these tests.

now it seems this test not doing anything right ? do you have idea what we should test, or should we remove it ?

I would breing everything back except for verify(mockOutputStream).flush();

I just modify the test, and add spy there when creating codeWriter object. Also, i put a new method so we can mock this part
doReturn(mockOutputStream).when(codeWriter).getWriterOutputStream(mockWriter);
please let me know if you have better approach on this.

Ok, great, I think that works.

johncarl81 · 2020-11-05T15:28:00Z

transfuse/pom.xml

@@ -53,8 +53,9 @@
        <dependency>
            <groupId>commons-io</groupId>
            <artifactId>commons-io</artifactId>
-            <version>2.4</version>
+            <version>2.5</version>


👍... there are a few more commons-io used in testing, could you upgrade those as well in this pass?

johncarl81 · 2020-11-05T15:30:26Z

transfuse-core/src/main/java/org/androidtransfuse/gen/FilerSourceCodeWriter.java

@@ -49,10 +50,8 @@ public OutputStream openBinary(JPackage jPackage, String fileName) throws IOExce
        //generate a source file based on package and fileName
        String qualified = toQualifiedClassName(jPackage, fileName);
        JavaFileObject sourceFile = filer.createSourceFile(qualified, originating.getOriginatingElements(qualified));
-
-        OutputStream os = sourceFile.openOutputStream();
+        OutputStream os = new WriterOutputStream(sourceFile.openWriter(), "UTF-8");


Pass in StandardCharsets.UTF_8 instead of the string?

ah yes, i just replace it

johncarl81 · 2020-11-05T15:30:35Z

transfuse-core/src/main/java/org/androidtransfuse/gen/FilerSourceCodeWriter.java

@@ -63,7 +62,7 @@ private String toQualifiedClassName(JPackage pkg, String fileName) {
    @Override
    public void close() throws IOException {
        for (OutputStream openStream : openStreams) {
-            openStream.flush();
+//            openStream.flush();


johncarl81 · 2020-11-05T15:31:52Z

transfuse-core/src/main/java/org/androidtransfuse/util/apache/commons/WriterOutputStream.java

+            }
+        }
+    }
+}


Go ahead and get rid of anything not used. Since we're using utf-8, the checkIbmJdkWithBrokenUTF16 will never execute, and most of the constructors can be combined into one.

done, i just removed it and combine the constructor.

…fuse-support, remove unused code on writeroutputstream

johncarl81 · 2020-11-05T17:02:17Z

transfuse-core/src/main/java/org/androidtransfuse/util/apache/commons/WriterOutputStream.java

@@ -0,0 +1,88 @@
+package org.androidtransfuse.util.apache.commons;


I'd stick this in org.androidtransfuse.gen next to FilerSourceCodeWriter

done, just moved next to FilerSourceCodeWriter

doniwinata0309 · 2020-11-06T11:34:17Z

transfuse-core/src/main/java/org/androidtransfuse/gen/FilerSourceCodeWriter.java

        openStreams.add(os);

        return os;
    }

+    public OutputStream getWriterOutputStream(Writer writer) {
+        return new WriterOutputStream(writer, StandardCharsets.UTF_8);


@johncarl81 using StandardCharsets.UTF_8 this actually red on my editor. it required
1.6
1.6
to use 1.7. However it compiled successfully. Should we just leave it, or update it to 1.7 ?

Hmm, too big of a chance for this... maybe just define the string as a constant in this class?

yes i changed it to Charset.forName("UTF-8") it should be equals to it.

johncarl81 · 2020-11-06T17:54:47Z

transfuse-core/src/main/java/org/androidtransfuse/gen/FilerSourceCodeWriter.java

- *
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>


Revert these.

doniwinata0309 · 2021-01-05T05:13:23Z

Hi do you still have any further concern about this PR ? thank you

johncarl81 · 2021-01-20T00:15:27Z

Nope, I'm ready to merge. I just haven't had a moment to focus on this. Do you need this and parceler updated and pushed out to maven central?

doniwinata0309 · 2021-01-20T09:27:56Z

thanks @johncarl81, yes would be nice if i can try it on stable version. But actually not that urgent, i guess it is up to you.

doni added 2 commits November 2, 2020 16:51

repalce output stream with writeroutputstream

fbb1b5f

remove unused variable

5fdd78b

doniwinata0309 closed this Nov 2, 2020

doniwinata0309 reopened this Nov 2, 2020

doniwinata0309 commented Nov 2, 2020

View reviewed changes

transfuse/src/test/java/org/androidtransfuse/gen/FilerResourceWriterTest.java Show resolved Hide resolved

doniwinata0309 changed the title ~~Replace output stream with writeroutputstream~~ Replace outputstream with writeroutputstream Nov 2, 2020

doniwinata0309 mentioned this pull request Nov 2, 2020

ParcelAnnotationProcessor.process take lot amount of time. johncarl81/parceler#392

Open

johncarl81 reviewed Nov 3, 2020

View reviewed changes

transfuse-core/pom.xml Show resolved Hide resolved

johncarl81 reviewed Nov 3, 2020

View reviewed changes

doni added 2 commits November 5, 2020 16:27

Copy WriterOutputStream locally and add charset

59b671c

add test scope on tranfuse/pom.xml

8355efd

johncarl81 reviewed Nov 5, 2020

View reviewed changes

Change to StandartCharsets, remove comments, update common-io on tran…

2e2f68d

…fuse-support, remove unused code on writeroutputstream

johncarl81 reviewed Nov 5, 2020

View reviewed changes

fix test and move writer to gen folder

b7b9748

doniwinata0309 commented Nov 6, 2020

View reviewed changes

doniwinata0309 requested a review from johncarl81 November 6, 2020 11:34

johncarl81 reviewed Nov 6, 2020

View reviewed changes

change standardCharsets to charset.forname

4e6e310

doniwinata0309 requested a review from johncarl81 November 23, 2020 06:24

johncarl81 approved these changes Jul 17, 2021

View reviewed changes

johncarl81 merged commit 217e00f into johncarl81:master Jul 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace outputstream with writeroutputstream #234

Replace outputstream with writeroutputstream #234

doniwinata0309 commented Nov 2, 2020 •

edited

Loading

johncarl81 Nov 3, 2020

doniwinata0309 Nov 5, 2020

johncarl81 Nov 3, 2020

doniwinata0309 Nov 5, 2020

johncarl81 Nov 5, 2020

doniwinata0309 Nov 5, 2020 •

edited

Loading

johncarl81 Nov 3, 2020

doniwinata0309 Nov 5, 2020

johncarl81 Nov 5, 2020

doniwinata0309 Nov 6, 2020

johncarl81 Nov 6, 2020

johncarl81 Nov 5, 2020

doniwinata0309 Nov 5, 2020

johncarl81 Nov 5, 2020

doniwinata0309 Nov 5, 2020

johncarl81 Nov 5, 2020

doniwinata0309 Nov 7, 2020

johncarl81 Nov 5, 2020

doniwinata0309 Nov 5, 2020

johncarl81 Nov 5, 2020

doniwinata0309 Nov 6, 2020

doniwinata0309 Nov 6, 2020

johncarl81 Nov 6, 2020

doniwinata0309 Nov 7, 2020

johncarl81 Nov 6, 2020

doniwinata0309 Nov 7, 2020

doniwinata0309 commented Jan 5, 2021

johncarl81 commented Jan 20, 2021

doniwinata0309 commented Jan 20, 2021

		@@ -0,0 +1,88 @@
		package org.androidtransfuse.util.apache.commons;

Replace outputstream with writeroutputstream #234

Replace outputstream with writeroutputstream #234

Conversation

doniwinata0309 commented Nov 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doniwinata0309 Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doniwinata0309 commented Jan 5, 2021

johncarl81 commented Jan 20, 2021

doniwinata0309 commented Jan 20, 2021

doniwinata0309 commented Nov 2, 2020 •

edited

Loading

doniwinata0309 Nov 5, 2020 •

edited

Loading